Skip to content

Enable source-build prebuilt detection#1821

Merged
sharwell merged 9 commits into
dotnet:mainfrom
MichaelSimons:prebuilt-detection
May 16, 2023
Merged

Enable source-build prebuilt detection#1821
sharwell merged 9 commits into
dotnet:mainfrom
MichaelSimons:prebuilt-detection

Conversation

@MichaelSimons

@MichaelSimons MichaelSimons commented Apr 25, 2023

Copy link
Copy Markdown
Member

Fixes #1809

There are some prebuilts remaining as indicated in SourceBuildrebuiltBaseline.xml that still need addressing.

@ghost ghost added the Community label Apr 25, 2023
@MichaelSimons

Copy link
Copy Markdown
Member Author

cc @dotnet/source-build-internal, @mmitche, @oleksandr-didyk

@mmitche mmitche left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think Microsoft.NET.Compilers.Toolset could be unified with the rest of the roslyn dependencies and that would eliminate a huge swath.

@ViktorHofer

Copy link
Copy Markdown
Member

I think Microsoft.NET.Compilers.Toolset could be unified with the rest of the roslyn dependencies and that would eliminate a huge swath.

Do we need the Microsoft.NET.Compilers.Toolset dependency in this repo at all? It might have been needed in the past but I wonder if we can these days just use the compiler that comes via the toolset.

@mmitche

mmitche commented Apr 28, 2023

Copy link
Copy Markdown
Member

I think Microsoft.NET.Compilers.Toolset could be unified with the rest of the roslyn dependencies and that would eliminate a huge swath.

Do we need the Microsoft.NET.Compilers.Toolset dependency in this repo at all? It might have been needed in the past but I wonder if we can these days just use the compiler that comes via the toolset.

Probably not, if the repo has UseToolMicrosoftNetCompilersToolset set to false/unset

@MichaelSimons

Copy link
Copy Markdown
Member Author

Probably not, if the repo has UseToolMicrosoftNetCompilersToolset set to false/unset

UseToolMicrosoftNetCompilersToolset does not appear to be set.

@MichaelSimons

Copy link
Copy Markdown
Member Author

I think Microsoft.NET.Compilers.Toolset could be unified with the rest of the roslyn dependencies and that would eliminate a huge swath.

The Microsoft.NET.Compilers.Toolset dependency is used to control all roslyn dependencies. Unfortunately the use of MicrosoftNetCompilersToolsetPackageVersion caused the repo to encounter dotnet/source-build#3392 which caused the repo to build with the MicrosoftNetCompilersToolsetPackageVersion coming from the SDK instead of the repo's versions.props.

@MichaelSimons MichaelSimons changed the title Initial enablement of source-build prebuilt detection Enable source-build prebuilt detection May 3, 2023
@MichaelSimons MichaelSimons requested a review from mmitche May 3, 2023 22:14
@MichaelSimons

Copy link
Copy Markdown
Member Author

@ViktorHofer and @mmitche - I has resolved all prebuilts except one that I propose leaving as an exception for now. Please review. TIA

Comment thread eng/Versions.props Outdated
Comment thread eng/Version.Details.xml
Comment on lines +68 to +48
<Dependency Name="Microsoft.SourceLink.GitHub" Version="8.0.0-beta.23218.3" CoherentParentDependency="Microsoft.DotNet.Arcade.Sdk">
<Uri>https://github.com/dotnet/sourcelink</Uri>
<Sha>47c52dd2ebf9edfd40abdcff999c13eb461f6ce2</Sha>
<SourceBuild RepoName="sourcelink" ManagedOnly="true" />
</Dependency>

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to remove these after P4 when sourcelink will be part of the sdk?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I must be living under a rock - this is news to me. If that's the case, then yes all sourcelink dependencies would have to be removed as well as removing the sourcelink repo from the VMR.

Can you point me to an issue for this work?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MichaelSimons

Copy link
Copy Markdown
Member Author

cc @sharwell - Can you review and merge if you approve? TIA.

Comment thread eng/Versions.props Outdated
@MichaelSimons

Copy link
Copy Markdown
Member Author

@sharwell - could you please review and merge if approved? This PR conflicts with every Arcade dependency flow so it is a burden to keep green. TIA

@sharwell sharwell merged commit d8962f6 into dotnet:main May 16, 2023
@ghost ghost added this to the Next milestone May 16, 2023
@MichaelSimons MichaelSimons deleted the prebuilt-detection branch May 16, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Enable source-build pre-built detection

4 participants